Skip to content

Mark instance in cgf_diagnostics::teardown() unavailable instead of resetting it - #339

Open
GagaLP wants to merge 1 commit into
masterfrom
fix/cgf-diagnostics-teardown
Open

Mark instance in cgf_diagnostics::teardown() unavailable instead of resetting it#339
GagaLP wants to merge 1 commit into
masterfrom
fix/cgf-diagnostics-teardown

Conversation

@GagaLP

@GagaLP GagaLP commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

After a while of debugging a strange Access Violation error on Windows during teardown of Celerity, I traced it back to cgf_diagnostics::teardown().

cgf_diagnostics stores its singleton in a thread_local std::unique_ptr. teardown() is called from runtime::impl's shutdown path, which runs when runtime::s_instance (a plain static member) is destroyed, and if the application never calls runtime::shutdown() explicitly, that happens as part of normal static-object teardown after main() returns.

The C++ standard leaves the relative order of static-object destruction and thread_local destruction for the main thread undefined. On Windows, these use separate mechanisms (the CRT's atexit/exit machinery vs. TLS callbacks), and in this case the thread_local storage for the main thread was already gone by the time teardown() called m_instance.reset() on it, resulting in access to unavailable memory.

Removing the m_instance.reset() call fixes this issue while keeping the logic the same: the whole point of using a smart pointer here is that its cleanup is already handled automatically, so an explicit reset in teardown() is redundant.

@GagaLP
GagaLP requested a review from PeterTh July 23, 2026 16:48
@github-actions

Copy link
Copy Markdown

Check-perf-impact results: (ae6918621b46271c2f10d6eb978fe95d)

❓ No new benchmark data submitted. ❓
Please re-run the microbenchmarks and include the results if your commit could potentially affect performance.

@PeterTh PeterTh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed this a lot offline and I think this is the best solution.

@GagaLP
GagaLP force-pushed the fix/cgf-diagnostics-teardown branch from f5df865 to 2667a57 Compare August 4, 2026 11:21
@GagaLP
GagaLP force-pushed the fix/cgf-diagnostics-teardown branch from 2667a57 to 2665f55 Compare August 4, 2026 11:56
@GagaLP GagaLP changed the title Make cgf_diagnostics::teardown() a no-op instead of resetting the instance Mark instance in cgf_diagnostics::teardown() unavailable instead of resetting it Aug 4, 2026
@coveralls

Copy link
Copy Markdown

Coverage Report for CI Build 30906911475

Coverage increased (+0.01%) to 95.079%

Details

  • Coverage increased (+0.01%) from the base build.
  • Patch coverage: 7 of 7 lines across 1 file are fully covered (100%).
  • No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 7264
Covered Lines: 7159
Line Coverage: 98.55%
Relevant Branches: 3690
Covered Branches: 3256
Branch Coverage: 88.24%
Branches in Coverage %: Yes
Coverage Strength: 2058390.61 hits per line

💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants